Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Suppress index-out-of-bounds warning from LGTM about loop unrolling #8380

Merged
merged 4 commits into from Sep 6, 2019
Merged

Suppress index-out-of-bounds warning from LGTM about loop unrolling #8380

merged 4 commits into from Sep 6, 2019

Conversation

asdf2014
Copy link
Member

Description

Suppress index-out-of-bounds warning from LGTM about loop unrolling

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths.
  • added integration tests.
  • been tested in a test Druid cluster.

@asdf2014 asdf2014 added the Area - Dev For items related to the project itself, like dev docs and checklists, but not CI label Aug 23, 2019
gianm
gianm reviewed Aug 23, 2019
View reviewed changes
processing/src/main/java/org/apache/druid/query/topn/TopNNumericResultBuilder.java Outdated
metricValues.put(aggFactoryNames[i + 5], metricVals[i + 5]);
metricValues.put(aggFactoryNames[i + 6], metricVals[i + 6]);
metricValues.put(aggFactoryNames[i + 7], metricVals[i + 7]);
metricValues.put(aggFactoryNames[i + 1], metricVals[i + 1]); // lgtm[java/index-out-of-bounds]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO would be nice to have a meta-comment here about why these are needed. Something like:

// LGTM.com flags this, but it's safe because we know "metricVals.length - extra" is a multiple of LOOP_UNROLL_COUNT.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's better. Thanks a lot.

@asdf2014
Copy link
Member Author

asdf2014 commented Sep 4, 2019

@gianm PTAL.

@himanshug himanshug merged commit 9fa3407 into apache:master Sep 6, 2019
@asdf2014 asdf2014 deleted the suppress_index_out_of_bounds branch September 7, 2019 05:43
@jon-wei jon-wei added this to the 0.17.0 milestone Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gianm gianm gianm left review comments

Assignees
No one assigned
Labels
Area - Dev For items related to the project itself, like dev docs and checklists, but not CI
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@asdf2014 @gianm @himanshug @jon-wei